Skip to content

fix docs format#2497

Merged
yihau merged 8 commits into
anza-xyz:masterfrom
yihau:fix-docs-2
Aug 11, 2024
Merged

fix docs format#2497
yihau merged 8 commits into
anza-xyz:masterfrom
yihau:fix-docs-2

Conversation

@yihau
Copy link
Copy Markdown
Member

@yihau yihau commented Aug 8, 2024

(part of #2487)

Problem

Screenshot 2024-08-06 at 23 56 02

docs take a stricter approach to indentation rules…

Summary of Changes

fix it

@yihau yihau requested a review from steviez August 8, 2024 16:36
@yihau yihau marked this pull request as ready for review August 8, 2024 16:36
steviez
steviez previously approved these changes Aug 8, 2024
Copy link
Copy Markdown

@steviez steviez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor nitpicks, but on second thought, maybe we shouldn't be making those changes in this PR. So, I'd say go ahead and ship it

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, the first line above the diff should probably have a period at the end
to a specific public key TO to a specific public key.

Comment thread zk-sdk/src/encryption/elgamal.rs Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, the first line above the diff should probably have a period at the end
to a specific public key TO to a specific public key.

Comment on lines 216 to 231
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an auto-generated file, so the next time we have to update it, there's a decent chance we hit this lint again 😕 Can we ignore the lint for this module?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good callout. Hypothetically, we could wait for that crate to update (assuming they pay attention to lints) or PR the crate ourselves

Copy link
Copy Markdown

@steviez steviez Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe we can ignore the lint for just this crate (like Tyera suggested above) ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry if I asked this before, but could you tell me how we generate those files? like what are the commands or libs we use? I guess it will be better if we can have a patch for the upstream 🤔

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, I removed this docs change from this PR: e01da6e looks like it's worth to have another PR for discussing this one haha. will ping you guys there!

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an auto-generated file, so the next time we have to update it, there's a decent chance we hit this lint again

It looks like this might be the case for the changes made in: #1516

I removed this docs change from this PR

This seems like a good compromise for this PR.

sorry if I asked this before, but could you tell me how we generate those files? like what are the commands or libs we use?

I believe this script will do it:
https://github.com/anza-xyz/agave/blob/master/storage-bigtable/build-proto/build.sh

Running that script + doing git diff, I see some items that were removed in 1516 are added again. Seeing this, I'm now more in favor of Tyera's suggestion for ignoring more lints for these couple files

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, I forgot to follow up on 1516 and re-add that stuff.
Yes, the link Steve posted is the correct build script. We are using tonic_build to generate the files from the googleapi protobuf files, so we commit new files when that crate updates. There are a couple doc blocks that we have been manually adding ignore to, although we could potentially make the docs CI steps ignore these files, or pick some other approach instead.

Anyway, thanks for removing the google file from this PR.

@yihau
Copy link
Copy Markdown
Member Author

yihau commented Aug 9, 2024

sorry, I think I committed some wrong indent and broke the original meaning. just read the docs again and pushed some updates. 🫠

@mergify
Copy link
Copy Markdown

mergify Bot commented Aug 9, 2024

The Firedancer team maintains a line-for-line reimplementation of the
native programs, and until native programs are moved to BPF, those
implementations must exactly match their Agave counterparts.
If this PR represents a change to a native program implementation (not
tests), please include a reviewer from the Firedancer team. And please
keep refactors to a minimum.

steviez
steviez previously approved these changes Aug 9, 2024
@yihau
Copy link
Copy Markdown
Member Author

yihau commented Aug 9, 2024

force-pushed for fixing the conflict 🫠

@yihau yihau merged commit 3838ee0 into anza-xyz:master Aug 11, 2024
@yihau yihau deleted the fix-docs-2 branch August 11, 2024 09:14
ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024
* fix docs format

* fix wrong indent

* fix wrong indent

* fix wrong indent

* fix wrong indent

* fix wrong indent

* fix wrong indent

* revert bigtable docs update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants